Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added NanoAOD to PromptReco #37728

Merged
merged 1 commit into from
May 2, 2022
Merged

Conversation

drkovalskyi
Copy link
Contributor

PR description:

Added NanoAOD option to the prompt reco configuration building to be used at Tier0. This PR is critical for NanoAOD integration at Tier0 and targets first collisions. Other developments outside CMSSW (in WMCore) are on hold till we have a working pre-release or IB to use in Tier0 replays.

PR validation:

  • Executed scram unit tests
  • Tested that one can create a working configuration that produces valid output

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37728/29578

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @drkovalskyi for master.

It involves the following packages:

  • Configuration/DataProcessing (operations)

@cmsbuild, @perrotta, @qliphy, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @fabiocos this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@perrotta
Copy link
Contributor

please test

@perrotta
Copy link
Contributor

Do I understand correctly that the unit tests should now pick up the new config, which will therefore get tested already in the IB tests then?

Do I understand correctly that this PR adds the nanoAODStep to the prompt reco by default?

You need eventually a backport to 12_3, I guess. As you know a 12_3_2 release was built just a few hours ago, mostly to satisfy the request from the T0 crew. What are the plans to deploy these further developments in the Tier0 release?

@drkovalskyi
Copy link
Contributor Author

The tests should run as part of TestConfigDP. They are just making configurations, they are not trying to run them though, but they would catch incorrect configuration parameters.

Regarding nanoAODStep it will be optional as all other steps are in principle. It's just a configuration building tool.

Regarding back-porting to 12_3, it can be done, but I don't think it will be necessary if 12_4 will be our production release for stable beams. We won't need it earlier and we are happy to test with IB/pre-releases for now.

@davidlange6
Copy link
Contributor

are there relvals/ (eg, runTheMatrix) workflows configured like this? cms should be testing the planned tier0 configuration..

@mariadalfonso
Copy link
Contributor

are there relvals/ (eg, runTheMatrix) workflows configured like this? cms should be testing the planned tier0 configuration..

@[davidlange6]
we have one that runs mini and nano with Run2 inputs (not in the default run3)
https://github.com/cms-sw/cmssw/blob/master/Configuration/PyReleaseValidation/python/relval_steps.py#L3297-L3305

we should eventually add more for run3

@drkovalskyi
Copy link
Contributor Author

drkovalskyi commented Apr 29, 2022

The only thing that is not being tested in RelVals or the matrix is the simultaneous running of reco, pat and nano. This would be good to test. I can add a test like that to the matrix.

@drkovalskyi
Copy link
Contributor Author

Thanks @mariadalfonso. It would be good to have a test that has all 3 components, i.e. reco, pat and nano.

@davidlange6
Copy link
Contributor

davidlange6 commented Apr 29, 2022 via email

@drkovalskyi
Copy link
Contributor Author

Yes, exactly the point. But its not just a run-without-crashing test that should be done here

There are many things that would be great to do. I would prefer to focus on the task at hands at the moment though. We can expand Tier0 testing in future.

@davidlange6
Copy link
Contributor

davidlange6 commented Apr 29, 2022 via email

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9f1e08/24328/summary.html
COMMIT: 5a4da5e
CMSSW: CMSSW_12_4_X_2022-04-28-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37728/24328/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 3 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3695434
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3695404
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 205 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@perrotta
Copy link
Contributor

perrotta commented May 2, 2022

@drkovalskyi @mariadalfonso should we expect such a test for Run3 to be added already to this PR, so that it can be tested (as working) at once with the new option?

@drkovalskyi
Copy link
Contributor Author

@perrotta Having all 3 steps in one test is just a nice to have thing, because RECO and PAT are used/tested routinely at Tier0. PAT and NANO are tested by X-POG and it's the most non-trivial part. Let's integrate the changes so that we can continue integration.

@davidlange6
Copy link
Contributor

davidlange6 commented May 2, 2022 via email

@perrotta
Copy link
Contributor

perrotta commented May 2, 2022

are there relvals/ (eg, runTheMatrix) workflows configured like this? cms should be testing the planned tier0 configuration..

@[davidlange6] we have one that runs mini and nano with Run2 inputs (not in the default run3) https://github.com/cms-sw/cmssw/blob/master/Configuration/PyReleaseValidation/python/relval_steps.py#L3297-L3305

we should eventually add more for run3

@mariadalfonso is that Run3 version of the workflow something that you can setup quickly? Do you think that it can be already included in this PR, to allow at least some initial "testability" for it?

@drkovalskyi
Copy link
Contributor Author

@perrotta Could we just turn the request for additional tests into a github issue and proceed with the integration of this PR? I don't see anything urgent about having extra test implemented now and Maria may have many other urgent things to work on. At the same time this PR holds us from integrating NanoAOD into prompt reco at Tier0.

@mariadalfonso
Copy link
Contributor

@perrotta @drkovalskyi
I think it's easier for me to add more WF Run3 mini/nano on a separate followup PR.
Also need to figure out the Run3 data we have for runTheMatrix/ Relvals .
MINI/NANO/both DQM Run2 is already there and tested at any PRs.

@perrotta
Copy link
Contributor

perrotta commented May 2, 2022

+1

  • The new option is added in view of further developments: some testing capability is expected to get provided in parallel
  • Standard workflows are not affected

@cmsbuild
Copy link
Contributor

cmsbuild commented May 2, 2022

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will be automatically merged.

@davidlange6
Copy link
Contributor

davidlange6 commented Oct 11, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants